-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix docs.rs build for datafusion-proto (hopefully) #10254
Conversation
@@ -17,7 +17,6 @@ | |||
|
|||
#[allow(clippy::all)] | |||
#[rustfmt::skip] | |||
#[cfg(not(docsrs))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was added in #3580 but now that the code is vendored (doesn't require running protoc at compile time) I think it should work
However, I don't really understand why it stopped working in the first place 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The last successful docs.rs build was DF 35 - I took a look at the commits in the proto directory between 35 and 36 and couldn't see anything obvious that would have caused this to fail.
Agree that we should give this a shot and see what happens in #10217
A command that tests this is: I took this from the docs.rs build logs. By running locally, I can confirm that this patch fixes the docs build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks @alamb
Thank you @leoyvens -- that is quite clever. |
Which issue does this PR close?
Closes #10163
Rationale for this change
Docs.rs is broken failed for https://docs.rs/crate/datafusion-proto/latest
The build has a log https://docs.rs/crate/datafusion-proto/37.0.0/builds/1179419 shows the error that the
generated
module isn't workingWhat changes are included in this PR?
Remove cfg to get working again (maybe)
Are these changes tested?
No -- I don't know how to test them. In the other hands the docs are broken so this isn't going to make them any more broken 🤷
Are there any user-facing changes?